[Variant] Preserve `Field` throughout Variant shredding flow #8673

friendlymatthew · 2025-10-21T15:37:30Z

Which issue does this PR close?

Related to [Variant] Overly aggressive inference of UUID values #8420
Related to [Variant] Support Uuid/FixedSizeBinary(16) shredding #8665

Rationale for this change

This PR preserves the typed_value's Field metadata. This way, we can check for extension types.

parquet-variant-compute/src/variant_array.rs

friendlymatthew · 2025-10-21T15:38:46Z

parquet-variant-compute/src/variant_array.rs

    value: Option<&BinaryViewArray>,
    index: usize,
 ) -> Variant<'a, 'a> {
-    let data_type = typed_value.data_type();
+    let (_typed_value_field, typed_value_column) = typed_value;


Now that we have the field information, we can check for extension types like: e21dc1b

friendlymatthew · 2025-10-21T15:39:14Z

cc @scovich @alamb @klion26

scovich

Thanks for taking a stab at this. I'm not immediately sure how to react, so I just left a couple high level comments while I stew on it more.

scovich · 2025-10-21T19:03:34Z

parquet-variant-compute/src/shred_variant.rs

            .typed_value_field()
            .unwrap()
+            .1


Just glancing at this code in isolation, I would guess that .0 is the field and .1 is the array?
But that requires knowing context (ie of this PR)

If this usage will show up often, is it worth returning a newtype instead of a tuple?

yes , please, let's return a type with real field names and documentation

scovich · 2025-10-21T19:04:32Z

parquet-variant-compute/src/variant_array.rs

+        let typed_value = if let Some(typed_value_array) = typed_value.clone() {
+            let field_ref = Arc::new(Field::new(
+                "typed_value",
+                typed_value_array.data_type().clone(),
+                true,
+            ));
+            builder = builder.with_field(field_ref.clone(), typed_value_array.clone());
+
+            Some((field_ref, typed_value_array))
+        } else {
+            None
+        };


nit: this is just Option::map (no ? to complicate things)

scovich · 2025-10-21T19:06:42Z

parquet-variant-compute/src/variant_array.rs

+        let typed_value = if let Some(typed_value_array) = typed_value.clone() {
+            let field_ref = Arc::new(Field::new(
+                "typed_value",
+                typed_value_array.data_type().clone(),
+                true,
+            ));
+            builder = builder.with_field(field_ref.clone(), typed_value_array.clone());
+
+            Some((field_ref, typed_value_array))


because of the dual typing (one in field and one in array), this code is technically no longer infallible -- the data types could disagree. Not sure the best way to address that issue as long as we're passing a full-blown field.

Also, this logic is duplicated with the variant array constructor above.

friendlymatthew · 2025-10-21T20:45:26Z

Thanks for taking a stab at this. I'm not immediately sure how to react, so I just left a couple high level comments while I stew on it more.

I think one thing I'm debating is the coupling between the typed_value array and field. If field is an optional metadata, then maybe we can split array and field into separate optional fields.

alamb · 2025-10-23T17:23:10Z

parquet-variant-compute/src/shred_variant.rs

            .typed_value_field()
            .unwrap()
+            .1


yes , please, let's return a type with real field names and documentation

alamb · 2025-10-23T17:28:33Z

parquet-variant-compute/src/variant_array.rs

@@ -708,6 +730,9 @@ impl From<ShreddedVariantFieldArray> for StructArray {
    }
 }

+pub type TypedValue = (FieldRef, ArrayRef);


This combination of an Array (which only has a DataType) and a Field (which has the DataType and metadata information) has come up recently upstream as @paleolimbot is working extension types through DataFusion as well in

Enable placeholders with extension types datafusion#17986

We are still looking for a good pattern to use.

I wonder if something like this is too crazy

/// A value that has associated Arrow field infomration struct TypedValue<T> { value: T, // could be ArrayRef, could be ScalarValue field: FieldRef, }

🤔

Probably struct TypedArrayRef { value: ArrayRef, field: FieldRef } is best for now (keeping the FieldRef-is-a-type bit opaque). Using FieldRefs as types in DataFusion has mostly led to a lot of cloned metadata and heterogeneity with respect to how the name/metadata/nullability should propagate (or not) but it does seem to be the path of least resistance.

Check for field metadata when accessing typed value

friendlymatthew

Self review

cc @scovich @klion26 @liamzwbao @alamb

friendlymatthew · 2025-11-17T18:45:32Z

parquet-variant-compute/src/variant_array.rs

+    // note: these methods below make me want to impl Array for TypedArrayRef...
+    pub fn slice(&self, offset: usize, length: usize) -> Self {
+        let Self { inner, field } = self;
+
+        Self {
+            inner: inner.slice(offset, length),
+            field: Arc::clone(field),
+        }
+    }
+
+    pub fn is_valid(&self, index: usize) -> bool {
+        self.inner.is_valid(index)
+    }


I'm curious what people think. Since TypedArrayRef owns the inner ArrayRef, it'll be pretty easy to get TypedArrayRef to also impl Array.

I don't have a strong opinion, but it feels weird to rewrite methods that exist on a trait

I also wonder if we want to find a better home for this struct. I suspect a TypedArrayRef would be useful in various parts of the library, not just the variant-related work 🤔

cc @paleolimbot

This definitely solves the problem of writing APIs that need to propgagate extension metadata without constantly pairing a FieldRef. It may be worth starting with the internal struct and the methods like you have here and moving to a trait when there's a case that arises elsewhere that also needs that concept? (I'm not very good at predicting where things should go inside the arrow-rs crates)

friendlymatthew · 2025-11-17T18:48:34Z

parquet-variant-compute/src/variant_array.rs

+impl PartialEq for TypedArrayRef {
+    #[allow(clippy::op_ref)]
+    fn eq(&self, other: &Self) -> bool {
+        &self.inner == &other.inner && self.field == other.field
+    }
+}


I was forced to add references to both operands (removing it fails to compile). Spent a lot of time this morning trying to fix/investigate this issue

Weirdly, using a tuple like I did in a prior iteration of this PR would compile 🤔

For those curious:

Here's a repro: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=c868bf8f2f8e2b699f9a84b55a89cd71

And here's a more reduced version: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=224ffe1076c24e1c8db5d4a8f53ca2fc

I have vague memories of hitting head-scratchy bugs in comparison-wielding code like this, and was informed there are actually rustc bugs involved. Might it be rust-lang/rust#31740?

friendlymatthew · 2025-11-17T18:49:31Z

parquet-variant-compute/src/variant_array.rs

+
+    #[test]
+    fn test_fixed_size_binary_without_uuid_extension() {


Here's a test that will err when trying to access a FixedSizeBinary(16) without a Uuid extension type

friendlymatthew · 2025-11-17T18:49:58Z

parquet-variant-compute/src/variant_array.rs

+    #[test]
+    fn test_fixed_size_binary_with_uuid_extension() {


Here's a test case that will pass since we properly annotated the Uuid extension type to its field metadata

friendlymatthew · 2025-11-19T15:44:26Z

@scovich @klion26 wdyt?

scovich · 2025-11-19T16:19:11Z

I hope to get to this soon, but am a bit buried at work this week and last...

scovich

Seems like a lot of potentially unnecessary churn?
If we can't eliminate it with Deref etc, might we consider a separate PR that separates the churn from functional changes?

scovich · 2025-11-19T22:22:37Z

parquet-variant-compute/src/variant_array.rs

+    // note: these methods below make me want to impl Array for TypedArrayRef...
+    pub fn slice(&self, offset: usize, length: usize) -> Self {


I don't think we could actually leverage Array::slice because it returns Arc<dyn Array>, which cannot be converted to TypedArrayRef?

But we could potentially impl Deref for TypedArrayRef with Target = dyn Array, to cover e.g. is_valid and the various array-casting calls in the shredding code?

scovich · 2025-11-19T22:23:37Z

parquet-variant-compute/src/variant_array.rs

-    let data_type = typed_value.data_type();
+    let TypedArrayRef {
+        inner: typed_value_array,
+        field: typed_value_field,
+    } = typed_value;
+
+    let data_type = typed_value_array.data_type();


Instead of all this churn, an impl Deref could make it transparent?
(see other comment)

scovich · 2025-11-19T22:24:56Z

parquet-variant-compute/src/variant_array.rs

+                typed_value_field.try_extension_type(),
+                Ok(arrow_schema::extension::Uuid)


aside: It seems like we had a TODO somewhere, to check for an extension type without having to actually allocate it? Or is this check super cheap and we don't care?

scovich · 2025-11-19T22:25:38Z

parquet-variant-compute/src/variant_get.rs

-    let Some(typed_value) = shredding_state.typed_value_field() else {
+    let Some(BorrowedTypedArrayRef {
+        inner: typed_value_array,
+        field: _typed_value_field,
+    }) = shredding_state.typed_value_field()
+    else {


again, Deref seems like a less invasive option here?

scovich · 2025-11-19T22:27:15Z

parquet-variant-compute/src/variant_get.rs

-            .with_field("metadata", Arc::new(metadata_array), false)
-            .with_field("value", Arc::new(value_array), true)
-            .with_field("typed_value", Arc::new(typed_value_struct), true)
+            .with_column_name("metadata", Arc::new(metadata_array), false)
+            .with_column_name("value", Arc::new(value_array), true)
+            .with_column_name("typed_value", Arc::new(typed_value_struct), true)


There's a fair bit of churn from this rename, and I'm not certain the new name is accurate, given that it specifies more than just the column name?

Is it actually bad to keep the original with_field name, given that its signature nearly matches Field::new? And the new method can keep its (also perfectly descriptive) name with_field_ref?

scovich · 2025-11-19T22:30:59Z

parquet-variant-compute/src/variant_array.rs

-            typed_value: self.typed_value_field(),
+            typed_value: self
+                .typed_value_field()
+                .map(|TypedArrayRef { inner, field }| BorrowedTypedArrayRef { inner, field }),


Does this merit an impl From?

klion26 · 2025-11-21T07:27:18Z

parquet-variant-compute/src/variant_array.rs

+            VariantArray::try_new(&struct_array).expect("should create variant array");
+
+        let res = variant_array.try_value(0);
+        assert!(res.is_err())


Do we need to assert the error message here?

klion26 · 2025-11-21T07:39:34Z

parquet-variant-compute/src/shred_variant.rs

        // // inspect the typed_value Field and make sure it contains the canonical Uuid extension type
        // let typed_value_field = variant_array
        //     .inner()
        //     .fields()
        //     .into_iter()
        //     .find(|f| f.name() == "typed_value")
        //     .unwrap();

        // assert!(
        //     typed_value_field
        //         .try_extension_type::<extension::Uuid>()
        //         .is_ok()
        // );



Do we need to uncomment this check in current PR?

github-actions bot added the parquet-variant parquet-variant* crates label Oct 21, 2025

friendlymatthew commented Oct 21, 2025

View reviewed changes

parquet-variant-compute/src/variant_array.rs Outdated Show resolved Hide resolved

friendlymatthew commented Oct 21, 2025

View reviewed changes

scovich reviewed Oct 21, 2025

View reviewed changes

alamb reviewed Oct 23, 2025

View reviewed changes

friendlymatthew mentioned this pull request Nov 3, 2025

[Variant] Support shred_variant for Uuids #8666

Merged

friendlymatthew force-pushed the friendlymatthew/drill-field branch from 28bd6bb to 398335b Compare November 3, 2025 17:59

scovich mentioned this pull request Nov 7, 2025

[Variant] Enforce shredded-type validation in shred_variant #8796

Merged

friendlymatthew closed this Nov 17, 2025

friendlymatthew force-pushed the friendlymatthew/drill-field branch from 398335b to ca4a0ae Compare November 17, 2025 16:53

friendlymatthew reopened this Nov 17, 2025

friendlymatthew force-pushed the friendlymatthew/drill-field branch 2 times, most recently from 3cf96c8 to bbb681c Compare November 17, 2025 17:49

This is a combination of 5 commits.

d96ef3d

Check for field metadata when accessing typed value

friendlymatthew force-pushed the friendlymatthew/drill-field branch from 091eb35 to d96ef3d Compare November 17, 2025 18:43

friendlymatthew commented Nov 17, 2025

View reviewed changes

alamb mentioned this pull request Nov 19, 2025

[Variant] Support Uuid/FixedSizeBinary(16) shredding #8665

Closed

scovich reviewed Nov 19, 2025

View reviewed changes

klion26 reviewed Nov 21, 2025

View reviewed changes

		// note: these methods below make me want to impl Array for TypedArrayRef...
		pub fn slice(&self, offset: usize, length: usize) -> Self {

		typed_value_field.try_extension_type(),
		Ok(arrow_schema::extension::Uuid)

[Variant] Preserve Field throughout Variant shredding flow #8673

Are you sure you want to change the base?

[Variant] Preserve Field throughout Variant shredding flow #8673

Conversation

friendlymatthew commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlymatthew commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlymatthew commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlymatthew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

friendlymatthew commented Nov 19, 2025

Uh oh!

scovich commented Nov 19, 2025

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

[Variant] Preserve `Field` throughout Variant shredding flow #8673

[Variant] Preserve `Field` throughout Variant shredding flow #8673

friendlymatthew commented Oct 21, 2025 •

edited

Loading

friendlymatthew commented Oct 21, 2025 •

edited

Loading